Skip to content

DOC: update the Period.days_in_month docstring #20297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Mar 12, 2018
Merged

DOC: update the Period.days_in_month docstring #20297

merged 3 commits into from
Mar 12, 2018

Conversation

Tushar987
Copy link
Contributor

@Tushar987 Tushar987 commented Mar 12, 2018 •
edited
Loading

  • PR title is "DOC: update the docstring"
  • The validation script passes: scripts/validate_docstrings.py <your-function-or-method>
  • The PEP8 style check passes: git diff upstream/master -u -- "*.py" | flake8 --diff
  • The html version looks good: python doc/make.py --single <your-function-or-method>
  • It has been proofread on language by another sprint participant
################################################################################
################### Docstring (pandas.Period.days_in_month)  ###################
################################################################################

Get the total number of days in the month that this period falls on.

Returns
-------
int

See Also
--------
Period.daysinmonth : Returns the number of days in the month.
DatetimeIndex.daysinmonth : Return the number of days in the month.
calendar.monthrange : Return a tuple containing weekday
    (0-6 ~ Mon-Sun) and number of days (28-31).

Examples
--------
>>> p= pd.Period('2018-2-17')
>>> p.days_in_month
28

>>> import datetime
>>> pd.Period(datetime.datetime.today(), freq= 'B').days_in_month
31

Handles the leap year case as well:

>>> p= pd.Period('2016-2-17')
>>> p.days_in_month
29

################################################################################
################################## Validation ##################################
################################################################################

Errors found:
	No extended summary found
  • Short summary summarises pretty much all the characteristics of this attribute.

IHackPy
IHackPy suggested changes Mar 12, 2018
View reviewed changes
pandas/_libs/tslibs/period.pyx Outdated
--------
Period.daysinmonth : Returns the number of days in the month.
DatetimeIndex.daysinmonth : Return the number of days in the month.
calendar.monthrange : Return a tuple containing weekday
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In See Also section : place of Returns use Get

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apart from this LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I will make the suggested changes. Thanks for the review.

@TomAugspurger TomAugspurger added Docs Datetime Datetime data dtype labels Mar 12, 2018
TomAugspurger
TomAugspurger reviewed Mar 12, 2018
View reviewed changes
pandas/_libs/tslibs/period.pyx Outdated
28

>>> import datetime
>>> pd.Period(datetime.datetime.today(), freq= 'B').days_in_month
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you avoid datetime.today()? This won't pass doctests, aside from ~half the months in the year :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah sure !! Please pardon my naivety and this is a lifetime learning for sure. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I just remove the second example because I couldn't think of any other way around of using this attribute different than the first example?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Period('2018-03-01').days_in_month should be fine.

TomAugspurger
TomAugspurger reviewed Mar 12, 2018
View reviewed changes
pandas/_libs/tslibs/period.pyx Outdated

See Also
--------
Period.daysinmonth : Returns the number of days in the month.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returns -> Return

pandas/_libs/tslibs/period.pyx Outdated
28

>>> import datetime
>>> pd.Period(datetime.datetime.today(), freq= 'B').days_in_month
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Period('2018-03-01').days_in_month should be fine.

pandas/_libs/tslibs/period.pyx Outdated

Examples
--------
>>> p= pd.Period('2018-2-17')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pep8: space before the =

pandas/_libs/tslibs/period.pyx Outdated

Handles the leap year case as well:

>>> p= pd.Period('2016-2-17')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space before the =

@Tushar987
Copy link
Contributor Author

@himanshuawasthi95 @TomAugspurger, Made the suggested changes. Please Review.

@TomAugspurger
Copy link
Contributor

@Tushar987 Did you push your changes with the PEP8 fixes (the spaces before the =, so p = ...)?

@TomAugspurger
Copy link
Contributor

Oh, also the datetime.today change. I'm guessing you just need to push :)

@Tushar987
Copy link
Contributor Author

@TomAugspurger, Sorry! forgot to push changes. Please review.

@TomAugspurger
Copy link
Contributor

Looks good, thanks!

@TomAugspurger TomAugspurger merged commit 6e88d3b into pandas-dev:master Mar 12, 2018
@jorisvandenbossche
Copy link
Member

Hmm, I also recently merged PR for daysinmonth: #20295
Probably we should try to consolidate both (and let them have a shared docstring)

@TomAugspurger TomAugspurger added this to the 0.23.0 milestone Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TomAugspurger TomAugspurger TomAugspurger left review comments

@IHackPy IHackPy IHackPy requested changes

Assignees
No one assigned
Labels
Datetime Datetime data dtype Docs
Projects
None yet
Milestone
0.23.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Tushar987 @TomAugspurger @jorisvandenbossche @IHackPy